fix(auto-approval): extract nested shell substitutions for allowlist - #2068

⚙️ Run configuration

• Source mode: Auto
• Repository: Zoo-Code-Org/Zoo-Code
• Branch: main
• File: .coderabbit.yaml
• Commit: 37b66fe
• Layers: Repository YAML
• Review profile: ASSERTIVE
• Plan: Advanced
• Run ID: 6dff5bec-f177-463a-b5e2-04dee490bd79

📥 Commits

Reviewing files that changed from the base of the PR and between 61bf7b9 and cdac899.

📒 Files selected for processing: 3

• src/core/auto-approval/__tests__/commands.spec.ts
• src/shared/__tests__/parse-command.spec.ts
• src/shared/parse-command.ts

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review.

⚠️ CI failures not shown inline: 2

GitHub Actions: Changed-code mutation testing / 0_mutation-diff.txt: fix(auto-approval): extract nested shell substitutions for allowlist

Conclusion: failure

View job details

##[group]Run BASE_SHA="$(git rev-parse "$HEAD_SHA^1")"
 �[36;1mBASE_SHA="$(git rev-parse "$HEAD_SHA^1")"�[0m
 �[36;1mnode scripts/stryker-diff.mjs ci --base "$BASE_SHA" --head "$HEAD_SHA"�[0m
 shell: /usr/bin/bash -e {0}
 env:
   PNPM_HOME: /home/runner/setup-pnpm/node_modules/.bin
   STORE_PATH: /home/runner/setup-pnpm/node_modules/.bin/store/v10
   HEAD_SHA: 6b9cbe8878198cc61fc341a3509fa87a1e86bd18
 ##[endgroup]
 Mutation gate failed: extension has 1281 changed executable lines (limit 500). Split the PR or obtain a maintainer-reviewed narrow exclusion.
 ##[error]Process completed with exit code 1.

GitHub Actions: Changed-code mutation testing / mutation-diff: fix(auto-approval): extract nested shell substitutions for allowlist

Conclusion: failure

View job details

##[group]Run BASE_SHA="$(git rev-parse "$HEAD_SHA^1")"
 �[36;1mBASE_SHA="$(git rev-parse "$HEAD_SHA^1")"�[0m
 �[36;1mnode scripts/stryker-diff.mjs ci --base "$BASE_SHA" --head "$HEAD_SHA"�[0m
 shell: /usr/bin/bash -e {0}
 env:
   PNPM_HOME: /home/runner/setup-pnpm/node_modules/.bin
   STORE_PATH: /home/runner/setup-pnpm/node_modules/.bin/store/v10
   HEAD_SHA: 6b9cbe8878198cc61fc341a3509fa87a1e86bd18
 ##[endgroup]
 Mutation gate failed: extension has 1281 changed executable lines (limit 500). Split the PR or obtain a maintainer-reviewed narrow exclusion.
 ##[error]Process completed with exit code 1.

🧰 Additional context used

📓 Path-based instructions: 4

Require regression coverage at the lowest valid harness with behavior-focused assertions, including relevant negative, error, false/unset, and boundary cases.

⚙️ CodeRabbit configuration file

Files:

• src/core/auto-approval/__tests__/commands.spec.ts

Check strict typing and exhaustive behavior across normal, boundary, error, cancellation, retry, and compatibility paths.

⚙️ CodeRabbit configuration file

Files:

• src/core/auto-approval/__tests__/commands.spec.ts

Verify extension/webview contracts, cancellation and error propagation, VS Code lifecycle correctness, and behavior under retries and partial failure.

⚙️ CodeRabbit configuration file

Files:

• src/core/auto-approval/__tests__/commands.spec.ts

Act as an adversarial second-opinion reviewer.

⚙️ CodeRabbit configuration file

Files:

• src/core/auto-approval/__tests__/commands.spec.ts

🔇 Additional comments: 2

src/core/auto-approval/__tests__/commands.spec.ts: 2

517-520: LGTM!

Also applies to: 522-528

725-735: LGTM!

Summary by CodeRabbit

• Bug Fixes
  • Shell command analysis now detects commands in command, process, and backtick substitutions, including nested cases.
  • Malformed syntax—including unclosed expansions, stray delimiters, NUL bytes, and excessive nesting—is reported as malformed instead of being partially parsed.
  • Heredoc parsing handles line endings, continuations, quoted delimiters, and tab-stripping rules more consistently.
  • Unbalanced delimiters inside quoted text remain valid when appropriate.
  • Malformed substitutions no longer expose trailing content as executable commands, and commands following substitutions are evaluated independently.

The shared shell parser now scans nested substitutions and heredocs, extracts inner commands, and reports structural errors. Auto-approval documentation and tests cover evaluation of extracted commands and fail-closed malformed_command decisions.

Changes: Shell command parsing and approval

Layer / File(s)	Summary
Track shell structure and malformed input src/shared/parse-command.ts, src/shared/__tests__/parse-command.spec.ts	The scanner tracks shell syntax and reports structural failures, including unclosed expansions, stray closers, NUL bytes, and excessive nesting.
Scan heredocs and expansion frames src/shared/parse-command.ts, src/shared/__tests__/parse-command.spec.ts	Heredoc parsing handles quoted delimiters, continuations, line endings, tab stripping, and nested expansions. Tests cover boundaries, masking, and malformed structures.
Parse host commands and substitution payloads src/shared/parse-command.ts, src/shared/__tests__/parse-command.spec.ts, webview-ui/src/utils/__tests__/command-parser.spec.ts, src/eslint-suppressions.json	parseCommand scans masked host text and recursively extracts substitution payloads. Tests cover nested substitutions and fallback behavior. The explicit-any suppression was removed.
Apply parser results to auto-approval decisions src/core/auto-approval/commands.ts, src/core/auto-approval/__tests__/commands.spec.ts	Documentation describes fail-closed handling for parser-detected structural errors. Tests cover extracted commands, executable tails, heredocs, and detailed malformed-command results.

Priority: ⬆️ High

Estimated code review effort: 4 (Complex) | ~60 minutes

Change: Bug fix · Severity of issue fixed: High

Sequence Diagram(s)

sequenceDiagram
  participant Input as Shell command
  participant Parser as parseCommand
  participant Approval as getCommandDecisionDetailed
  participant Policy as Allowlist rules
  Input->>Parser: Scan syntax and extract commands
  Parser->>Approval: Return commands or structural error
  Approval->>Policy: Evaluate extracted commands
  Policy->>Approval: Return decision

Loading

The reviewed parser changes no longer leave the identified command-approval bypasses unresolved.

Passed checks: 6 passed

Check name	Status	Explanation
Title check		The title clearly identifies the primary change: extracting nested shell substitutions for auto-approval allowlist handling.
Description check		The description explains the issue, implementation, scope, limitations, linked issue, and test results in detail. It does not reproduce the template headings or include the pre-submission checklist, d…
Linked Issues check		Issue #1947 is an active direct link. parse-command.ts replaces regex splitting with a quote-aware nesting scan. It recursively extracts nested command, process, parameter, arithmetic, and unquoted-…
Out of Scope Changes check		The production scanner change, approval integration, parser tests, webview regression tests, documentation updates, and lint-suppression cleanup support Issue #1947. The changes preserve matching sema…
Persistence Integrity		No changed persistence path exists. The PR changes src/shared/parse-command.ts parsing logic, src/core/auto-approval/commands.ts documentation, tests, and an ESLint suppression. The changed source…
Lifecycle Resource Cleanup		No changed lifecycle path exists. The PR changes synchronous parsing in src/shared/parse-command.ts and documentation in commands.ts; the only runtime import is shell-quote. The changed files ad…

Full details: Description check

Explanation

The description explains the issue, implementation, scope, limitations, linked issue, and test results in detail. It does not reproduce the template headings or include the pre-submission checklist, documentation section, or contact field, but the required technical information is mostly complete.

Full details: Linked Issues check

Explanation

Issue #1947 is an active direct link. parse-command.ts replaces regex splitting with a quote-aware nesting scan. It recursively extracts nested command, process, parameter, arithmetic, and unquoted-heredoc substitutions. commands.ts preserves existing matching and returns malformed_command for parser errors. The added approval and parser tests cover the reported bypasses, quoted heredocs, malformed delimiters, NUL input, and depth limits. The documented double-quote and heredoc-opener limitations match the issue scope.

Full details: Out of Scope Changes check

Explanation

The production scanner change, approval integration, parser tests, webview regression tests, documentation updates, and lint-suppression cleanup support Issue #1947. The changes preserve matching semantics and add regression coverage for the new parsing behavior. No unrelated product change is shown.

Full details: Persistence Integrity

Explanation

No changed persistence path exists. The PR changes src/shared/parse-command.ts parsing logic, src/core/auto-approval/commands.ts documentation, tests, and an ESLint suppression. The changed source imports only shell-quote or parse-command and contains no storage, file/database write, rollback, atomic-write, or awaited persistence operation. Therefore no persistence loss or corruption scenario is introduced.

Full details: Lifecycle Resource Cleanup

Explanation

No changed lifecycle path exists. The PR changes synchronous parsing in src/shared/parse-command.ts and documentation in commands.ts; the only runtime import is shell-quote. The changed files add no listeners, watchers, subscriptions, timers, tasks, providers, workers, or disposal paths. The remaining changes are tests and lint configuration.

Explanation

The changed parser still bypasses approval for command substitutions inside double quotes. In src/shared/parse-command.ts, QUOTE_FRAME_KINDS includes double, and the top-level close path stores the complete quoted region as one opaque Q placeholder without scanning its contents (lines 1290-1297). Therefore echo "$(rm -rf /)" remains one host command. With allowedCommands: ["echo"], getSingleCommandDecision approves it by prefix (lines 335-362), while the shell still executes rm -rf /. This path was changed by the parser rewrite; the base implementation extracted $() before masking double quotes (base lines 622-650).

Resolution

Update src/shared/parse-command.ts to extract and independently re-scan command substitutions inside double-quoted regions while preserving double-quote grouping and operator handling. Add parser and auto-approval regression tests proving getCommandDecision('echo "$(rm -rf /)"', ['echo']) is not auto_approve.

Explanation

The parser change alters the durable CommandExecution command-pattern surface: CommandExecution.tsx:72-99 calls the changed parseCommand, and the real CommandPatternSelector renders the resulting patterns. The PR adds focused parser and auto-approval tests, but it adds no Playwright component test or snapshots for CommandExecution with a nested or parameter-expansion command. The existing visual fixture uses fixed patterns in ThemeSensitiveStatus.visual.fixture.tsx:15-24, and the base/head visual inventories are identical. The existing CommandExecution.spec.tsx mocks CommandPatternSelector and uses loose text assertions, so it does not provide the required durable UI regression evidence.

Resolution

Add a Playwright component visual test for the real CommandExecution command-pattern surface. Render a nested or parameter-expansion command, verify the inner pattern is visible, exercise allowed and denied selector states, and commit snapshots for dark, light, high-contrast, and high-contrast-light themes.

🛠️ Fix failing CI checks: 💡

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Create a new PR

• X
• Mastodon
• Reddit
• LinkedIn

🏁 Script executed:

#!/bin/bash
set -euo pipefail
gh api repos/Zoo-Code-Org/Zoo-Code/pulls/2068 --jq '{body,base:.base.sha,head:.head.sha}'
gh api --paginate repos/Zoo-Code-Org/Zoo-Code/issues/2068/comments --jq '.[] | {id,user:.user.login,body}'
gh api --paginate repos/Zoo-Code-Org/Zoo-Code/pulls/2068/comments --jq '.[] | {id,user:.user.login,path,line,body,in_reply_to_id}'
ast-grep outline src/shared/parse-command.ts
rg -n -C 4 'stray-closer|case x in|depth.?0|double.quote|Known limitations' src/shared/parse-command.ts src/shared/__tests__/parse-command.spec.ts src/core/auto-approval/__tests__/commands.spec.ts
git show 8018cba40:src/shared/parse-command.ts | head -n 180

Length of output: 151523

🏁 Script executed:

#!/bin/bash
set -euo pipefail
printf '%s\n' '--- base parser implementation ---'
git show 8018cba40:src/shared/parse-command.ts | nl -ba | sed -n '560,810p'
printf '%s\n' '--- current quote handling ---'
sed -n '970,1070p' src/shared/parse-command.ts
printf '%s\n' '--- approval decision implementation map ---'
ast-grep outline src/core/auto-approval/commands.ts
sed -n '225,350p' src/core/auto-approval/commands.ts

Length of output: 18303

Learnt from: DaubnerF
URL: https://github.com/Zoo-Code-Org/Zoo-Code/pull/2068

Timestamp: 2026-10-21T13:52:31.352Z
Learning: For PR #2068 in Zoo-Code-Org/Zoo-Code, src/shared/parse-command.ts intentionally treats depth-0 `)` as host text to preserve case-pattern parsing, as requested in discussion_r4319968449. Substitutions after that closer must still be extracted. Double-quoted command-substitution opacity is a documented, pre-existing limitation: base 8018cba40 masks the substitution placeholder inside a quote placeholder and restores it without independent evaluation. Do not classify that unchanged limitation as a regression introduced by this PR.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

Comments resolved and changes approved.

Comments resolved and changes approved.